Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Proxy mode for FlowAggregator #6920

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

antoninbas
Copy link
Contributor

In Proxy mode, connection data is not buffered in the FlowAggregator and records from source and destination Nodes are not correlated. Instead, records are proxied directly to the destination collector. In Proxy mode, only the IPFIX exporter is supported.

A simple e2e test is added to test this new mode.

Fixes #6773

@@ -100,7 +100,7 @@ func makeDefaultElementsWithValue(infoElements []*entities.InfoElement) ([]entit
return elementsWithValue, nil
}

func newPreprocessor(infoElementsV4, infoElementsV6 []*entities.InfoElement, inCh <-chan *entities.Message, outCh chan<- entities.Record) (*preprocessor, error) {
func newPreprocessor(infoElementsV4, infoElementsV6 []*entities.InfoElement, inCh <-chan *entities.Message, outCh chan<- *entities.Message) (*preprocessor, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tnqn I had to revert from a Record output channel back to a Message output channel. I wanted the ability to preserve message header information (observation domain id, exporter address) so that the information can be added to proxied records using the standard originalX information elements (e.g. originalObservationDomainId).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

@antoninbas antoninbas force-pushed the fa-add-proxy-mode branch 2 times, most recently from 4722a39 to 79d8f6d Compare January 14, 2025 18:12
build/charts/flow-aggregator/conf/flow-aggregator.conf Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator.go Show resolved Hide resolved
pkg/flowaggregator/flowaggregator.go Outdated Show resolved Hide resolved
pkg/flowaggregator/flowaggregator.go Show resolved Hide resolved
In Proxy mode, connection data is not buffered in the FlowAggregator and
records from source and destination Nodes are not correlated. Instead,
records are proxied directly to the destination collector. In Proxy
mode, only the IPFIX exporter is supported.

A simple e2e test is added to test this new mode.

Fixes antrea-io#6773

Signed-off-by: Antonin Bas <[email protected]>
@antoninbas antoninbas force-pushed the fa-add-proxy-mode branch 2 times, most recently from 9d17e74 to 6f4d437 Compare January 17, 2025 19:10
@antoninbas antoninbas requested a review from luolanzone January 17, 2025 19:10
Signed-off-by: Antonin Bas <[email protected]>
@luolanzone luolanzone added this to the Antrea v2.3 release milestone Jan 21, 2025
cpInput.NumExtraElements += len(infoelements.AntreaSourceStatsElementList) + len(infoelements.AntreaDestinationStatsElementList) +
len(infoelements.AntreaFlowEndSecondsElementList) + len(infoelements.AntreaThroughputElementList) + len(infoelements.AntreaSourceThroughputElementList) + len(infoelements.AntreaDestinationThroughputElementList)
} else {
// originalObservationDomainId, originalExporterIPv4Address, originalExporterIPv6Address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that there was an issue and commit for removing these extra elements before: #2361, I am wondering will the old issue still valid? or those were added by aggregator and won't happen on proxy mode?
According to the issue, originalExporterIPv4Address/originalExporterIPv6Address are Node antrea-gateway IPs? How could user to know Node IPs and gateway IPs mapping? or it's user's responsibility and no way to do such correlation from Antrea aggregator?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose there will be a document update for these new elements and proxy mode in the following PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields don't make sense in aggregation mode because we are "merging" 2 records. The issue mentions this:

One option is to remove this as the original exporter IP does not make sense when correlating the flow records. The second option is to add the exporter IP in the flow record sent by the Flow Exporter and have two different fields corresponding to each exporter (source and destination).

This was the true problem IMO, not the fact that we use the gateway IP. Having these fields be a "private" overlay IP is fine IMO, as long as it is unique for a given cluster.

In proxy mode, there is no aggregation, so the fields "make sense".

Now these fields may not be the most useful to have, and they do add a few bytes to each record. But I would say it's pretty standard to have them and they can be used as a unique identifier for the "source" exporter.

I suppose there will be a document update for these new elements and proxy mode in the following PR?

Yes I will be able to add new fields to the documentation before we release Antrea v2.3.

fa.fillK8sMetadata(sourceAddress, destinationAddress, record, startTime)
}
fa.fillPodLabels(sourceAddress, destinationAddress, record, startTime)
if err := fa.fillClusterID(record); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be the same for all records in one cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but sending "common" data separately to avoid the duplication and reduce the size of records requires using IPFIX option templates. We don't have support for them in go-ipfix yet. It's on my roadmap (I can open an issue for it), but is not high priority IMO.

pkg/flowaggregator/flowaggregator.go Show resolved Hide resolved
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -100,7 +100,7 @@ func makeDefaultElementsWithValue(infoElements []*entities.InfoElement) ([]entit
return elementsWithValue, nil
}

func newPreprocessor(infoElementsV4, infoElementsV6 []*entities.InfoElement, inCh <-chan *entities.Message, outCh chan<- entities.Record) (*preprocessor, error) {
func newPreprocessor(infoElementsV4, infoElementsV6 []*entities.InfoElement, inCh <-chan *entities.Message, outCh chan<- *entities.Message) (*preprocessor, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas antoninbas merged commit 9e5113d into antrea-io:main Jan 27, 2025
55 of 62 checks passed
@antoninbas antoninbas deleted the fa-add-proxy-mode branch January 27, 2025 18:09
@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FlowAggregator] Add "proxy" mode
3 participants